filter_state: remove checks for read-only#44343
filter_state: remove checks for read-only#44343ggreenway wants to merge 4 commits intoenvoyproxy:mainfrom
Conversation
The checks for mutable vs read-only have caused many subtle runtime bugs, and have not provided much benefit. The parameter for read-only vs mutable is now ignored, and will be removed from the codebase in phases. Signed-off-by: Greg Greenway <ggreenway@apple.com>
Signed-off-by: Greg Greenway <ggreenway@apple.com>
|
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to |
adisuissa
left a comment
adisuissa
left a comment
There was a problem hiding this comment.
From a high-level the direction looks good to me, but just wanted to clarify that it is unclear to me why in #4820 it was decided to add the mutable state, instead of just changing it to always be mutable. I don't have sufficient context on this.
|
I did some archeology, and found the background on the original decision to make the data read-only: #3918 (comment). I don't think that applies to how we're using it at this point; I think ReadOnly is causing more harm than good in our current implementation and use patterns. |
Signed-off-by: Greg Greenway <ggreenway@apple.com>
adisuissa
left a comment
adisuissa
left a comment
There was a problem hiding this comment.
Thanks, LGTM!
My understanding is that this read-only mode was the one that was initially used, because the idea was that filters will communicate state and no other filter in their path will modify that info. I guess this is no longer true.
Left a couple of points to think on if this needs further work.
| * (mutable and readOnly, or readOnly and mutable) or different life_span. | ||
| * This is to enforce a single authoritative source for each piece of | ||
| * data stored in FilterState. | ||
| * Note that it is an error to call setData() twice with the same data_name, but different |
There was a problem hiding this comment.
Not directly related to this PR, but I wonder if this is a similar kind of constraint that should be addressed by supporting life-span "upgrade".
| void setData(absl::string_view data_name, std::shared_ptr<Object> data, StateType /*state_type*/, | ||
| LifeSpan life_span = LifeSpan::FilterChain, | ||
| StreamSharingMayImpactPooling stream_sharing = StreamSharingMayImpactPooling::None) { | ||
| return setData(data_name, std::move(data), life_span, stream_sharing); |
There was a problem hiding this comment.
Is there a way to note that the wrong state-type (readonly) is invoked here?
Maybe not now, as it will cause lots of changes, but if there's an assert failure here, the wrong usage will be detected in the tests. We may be able to convert all the uses gradually, add the assert, and then remove this function.
2 participants
The checks for mutable vs read-only have caused many subtle runtime bugs, and have not provided much benefit.
The parameter for read-only vs mutable is now ignored, and will be removed from the codebase in phases.
Commit Message:
Additional Description:
Risk Level: Low
Testing: Updated tests
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]